Skip to content

Improve planning and continue cleaning - #38

Open
bobjansen wants to merge 24 commits into
mainfrom
better-plans
Open

Improve planning and continue cleaning#38
bobjansen wants to merge 24 commits into
mainfrom
better-plans

Conversation

@bobjansen

Copy link
Copy Markdown
Owner

No description provided.

@bobjansen bobjansen self-assigned this Aug 29, 2026
bobjansen and others added 22 commits August 29, 2026 19:52
explain_physical now prints the fallback root's node kind alongside the
reason (MaterializedCall(Melt: root is not a row-local map)), matching the
physical_plan.hpp contract that the line names the logical subtree the
fallback keeps. Adds plans/physical-fallback-adapter-plan.md for the
remaining Phase 5 item 3 work.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
build_operator_impl's per-kind switch carried 15 branches (Columns, Melt,
Dcast, Cov, Corr, Transpose, materializing Join, Matmul, Update, Resample,
Window, AsTimeframe, Model, Construct/Stream, Program) that each rebuilt a
child through build_operator, materialized it, and called a table
function. Every one of those kinds already has an equivalent branch in
interpret_node, which recurses through itself -- so the switch was a
second copy of that dispatch, kept in sync by hand.

Delete the branches and let the fallback tail (interpret_node +
make_table_source) handle them. plan_physical does not migrate these
shapes, so they are still counted by note_materialized_call, and
physical_fallbacks_for(kind) now buckets the whole backlog by node kind.
A fallback subtree is planned once at its root rather than re-entering
plan_physical per node.

Trade-off: the input side of these breakers no longer runs through the
fused parallel scan-filter physical path (interpret_node copies whole
tables and filters serially). No PDS-H or ClickBench query hits bucket A
-- q13's left join is FusedLeftJoinCount, a migrated path -- so this is
unmeasured on the benchmarked shapes. Recoverable per kind by lifting it
to a migrated breaker-over-pipeline the way Join / Aggregate / Order were;
the per-kind fallback counter is what will drive that.

build_binary_materializing_operator had no other caller and is removed.
All 1,815 non-slow tests pass; strict GCC build and Debug/Release plugin
builds pass. chunked.cpp -356 lines.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Routing every materializing breaker straight through interpret_node
(previous commit) re-evaluated its inputs whole-table and serial:
join_filter_rank regressed +14.7% because the filter feeding its
grouped-rank update lost the fused parallel scan build_operator gives it.

build_operator_impl's fallback tail now calls build_materialized_fallback,
which builds each of the node's relational inputs via build_operator +
materialize_operator, then runs interpret_node over the node with those
inputs handed back through a new ExecutionContext::pre_materialized_children
list (node ptr -> table). interpret_node checks the list at entry and
returns the pre-built table instead of recursing; only direct inputs are
listed, so the node itself and anything deeper interpret normally.

fallback_relational_inputs is an explicit switch allowlist. Window's
direct child is an `update` clause (interpret_node must own it, with the
window duration) so its grandchild is the input; Stream / Construct /
Program and unlisted kinds get no pre-build (their children are template
or expression nodes). A first cut without those exclusions failed 51
tests by evaluating an update clause or a __stream_input__ transform
standalone.

A/B vs pre-step-2 (c9a20fb1) over join,reshape,window,stats,transform,
multi, 12 interleaved repeats: every query noise, join_filter_rank -3.8%,
total +0.9%. All 1,815 non-slow tests pass; strict GCC and Debug/Release
builds pass.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Whole-tree `scripts/clang-format.sh -i` over src/include/tests/tools.
Mechanical only. The drift is almost entirely from the pipeline-extraction
commits (0d60d634 and earlier), which were not format-checked at commit
time; this session's fallback-adapter change added one wrapped line in
interpreter.hpp. format-check CI runs the whole tree, so it gates on all
of it.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
bobjansen and others added 2 commits August 29, 2026 20:38
The pipeline-extraction commits (0d60d634 and earlier) landed without
passing the changed-lines clang-tidy gate; because the push presents the
new files (aggregate_chunked / join_chunked / pipeline_executor /
physical_executor) as all-new, the gate now surfaces their whole backlog.

Fixes:
- bugprone-unchecked-optional-access: the AggregateColumnMapping /
  JoinColumnMapping optionals are bound before first use (bind_* returns
  nullopt only once bound; the probe binds in setup_right_emit_schema).
  Access them through .value() so the invariant is checked, not assumed;
  inner std::optional<size_t> aggregate-input slots likewise (guarded to
  non-Count aggregations).
- bugprone-exception-escape: wrap the exit-time stats Reporter's stderr
  write in try/catch so a failed write can't std::terminate.
- bugprone-optional-value-conversion: return the std::optional<Chunk>
  directly instead of rebuilding it from a dereference.
- modernize-use-emplace / use-auto, performance-move-const-arg (std::move
  on a trivially-copyable value param), misc-const-correctness,
  cppcoreguidelines-missing-std-forward, readability-redundant-declaration.
- misc-use-internal-linkage: physical_executor.cpp and pipeline_executor.cpp
  now include their own *_internal.hpp (also catches definition/declaration
  drift); the three chunked.cpp parallelism helpers that are genuinely
  TU-local move into an anonymous namespace. The check is then disabled in
  .clang-tidy -- it can't see cross-TU use of the shared *_detail helpers
  and the remaining local helpers interleaved in those blocks would each
  need call-site requalification; it finds no bugs.

All 1,815 non-slow tests pass under Clang and strict GCC; Debug + Release
plugin builds pass; tree is clang-format clean.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
clang-tidy 18/20 (CI) flags std::optional::value() as an unchecked access
when it can't prove the optional is engaged -- clang-tidy 23 (local) does
not, so the previous commit's .value() calls passed here but failed CI.

Add explicit has_value() guards in the aggregate and join operator
methods that read columns_ (they run only after bind_aggregate_columns /
setup_right_emit_schema has bound it; the guard makes that a checked
error rather than an assumed invariant), and check the inner
aggregate_inputs[i] optional before dereferencing it (nullopt only for
Count, already skipped, but clang-tidy can't correlate that). Also
re-applies the pipeline_executor performance-move-const-arg fix dropped
in an earlier revert.

Verified with clang-tidy 20.1.0 via the CI clang-tidy-diff path: no
warnings on the changed lines. All 1,815 non-slow tests pass under Clang
and strict GCC; format clean.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant